Protocol-relative links for minify endpoints, fixes #50 #54
Conversation
| @@ -512,7 +512,7 @@ static function get_dependency_minified_url( array $deps, $type ) { | |||
| $ver_hash, | |||
| $type === 'scripts' ? 'js' : 'css', | |||
| ) ); | |||
| return $src; | |||
| return preg_replace( "(https?:)", "", $src ); | |||
There was a problem hiding this comment.
@lkraav how about:
return preg_replace( '#^https?:(?=//)#', '', $src );There was a problem hiding this comment.
The # are apparently a regex begin and end markers (googled), first I've seen them. I think they're OK. Regex start ^ also good. Single quotes OK, I forgot to check the coding style on that.
I'm not sure I'm versed enough in regex to quickly understand what the tailing group capture (?=//) wants to do. Can you explain?
There was a problem hiding this comment.
It's a lookahead group, making sure that // will be the first part of the resulting string after replacement.
There was a problem hiding this comment.
Sounds fancy and smart :) if tests show the resulting $src-s are correct, why not.
Do you want me to amend the PR or what should happen next? Seems like we have the solution.
There was a problem hiding this comment.
Please do amend the PR. Thanks!
we are currently using get_option( "home" ) to build the $src address. wordpress does not support protocol relative home address still as of 3.9, see https://core.trac.wordpress.org/ticket/21153
|
EDIT I tested the enhanced regex and it's creating good URLs. Maybe this can make for a 0.9.9? I noticed 1.0 had a lot of changes and I know you don't have a lot of time. It would be nice to get all my customers SSL-ready with this, though. |
|
A better solution is to actually use the home_url() function as follows: Protocol relative urls are unfortunately not fool proof and I have had problems with them in the past. Its better to always use https: on a resource include even on http: pages, if ssl is available, than a protocol relative url. |
|
site_url() and home_url() seem to be exactly the same thing in this scenario. @msigley the main problem here is how do you know if SSL is available if the WP home/site URLs are set to "http", but the site is loaded over "https"? If you force https and it isn't configured, that's a dead broken request right there. It's a pretty vague claim of "had problems in the past" IMHO and should require significantly more proof to influence @westonruter decision here. So far I see nothing that trumps the protocol-relative approach. |
|
@lkraav a more recent example I had of the downside of protocol-relative urls was including a javascript file from Authorize.net on one of my websites. The browser was trying to request the http version first, which wasn't available, then requested the https version which was. All of this added significant load time to the page. |
|
I don't think it is going to matter if the URLs being returned retain the protocol or if they are protocol-less. The URLs are only for the minified scripts on the same domain as the site you're on, and so of course HTTPS will be available if the page itself is loading over HTTPS. It's not manipulating the src for scripts on other domains. So probably #69 is going to be a better bet, though it should use |
|
Actually, see ad5eb95:
It seems that using |
we are currently using get_option( "home" ) to build the $src address.
wordpress does not support protocol relative home address still as of 3.9,
see https://core.trac.wordpress.org/ticket/21153